Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust maxAge minimum & documentation #188

Merged
merged 15 commits into from
May 7, 2024
Merged

Adjust maxAge minimum & documentation #188

merged 15 commits into from
May 7, 2024

Conversation

bigludo7
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #184

Special notes for reviewers:

Changelog input

 release-note
- Remove minimum 60 seconds to maxAge.
  -  Absence of maxAge means "any age"
  -  maxAge=0 means a fresh calculation.

Additional documentation

This section can be blank.

docs

update maxAge behaviour
Update maxAge behavior for Location Verfication yaml
@bigludo7 bigludo7 requested a review from alpaycetin74 April 23, 2024 15:35
@bigludo7 bigludo7 requested a review from jlurien as a code owner April 23, 2024 15:35
Copy link

github-actions bot commented Apr 23, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.04s
✅ OPENAPI spectral 3 0 4.52s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY secretlint yes no 0.72s
✅ YAML yamllint 3 0 0.56s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

code/API_definitions/location-verification.yaml Outdated Show resolved Hide resolved
@@ -37,6 +37,10 @@ info:

* **Max Age**: Maximum age of the location information which is accepted for the location retrieval (in seconds).

* Absence of maxAge means "any age" is acceptable for the client. In other words, this is like maxAge=infinite
* maxAge=0 means a fresh calculation is requested by the client.
If the system does not support it, or fresh location cannot be calculated at that time for any reason, the API response will be "unknown".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If the system does not support it, or fresh location cannot be calculated at that time for any reason, the API response will be "unknown".
If the system does not support it, or fresh location cannot be calculated at that time for any reason, the API response will be "UNKNOWN".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlurien I have an issue here because in location-retrieval we do not have unknown/UNKNOWN.
We have either

{
  "status": 400,
  "code": "LOCATION_RETRIEVAL.MAXAGE_INVALID_ARGUMENT",
  "message": "maxAge threshold cannot be satisfied"
}

or

{
  "status": 404,
  "code": "NOT_FOUND",
  "message": "The specified resource is not found"
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

400 seems a better fit because it specifically points to maxAge. And it applies for all maxAge values, not only 0.
If maxAge was not provided in the request (any age is acceptible), but the system still cannot find a location, 404 can be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alpaycetin74
Works for me and I can update the documentation accordingly.
If not able to provide require freshness (maxAge filled) --> 400 + code LOCATION_RETRIEVAL.MAXAGE_INVALID_ARGUMENT
If not able to provide any location for a request without maxAge --> 404

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't realize that.
Regarding errors, to me 400 is for cases when the provided input does not comply with the schema or some constraint that should be known by the client, independently of the device. It could fit for cases where some implementation does not support maxAge=0 for any user (it is still pending how to provide information about limits of the implementation to the client). But for cases where the maxAge cannot be satisfied for a specific device or at a specific moment, I think it is more of a 404 or even 422 (the entity combining device and maxAge is not processable by the system).

These RESTful interpretations of HTTP statuses are very subtle anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For (2) I think 404 is more natural, the (3) is more about the combination of device + maxAge at a given time

(1)If maxAge input does not fit the pattern (like 5fr/"3) --> 400
(2)If not able to locate the mobile --> 404 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND
(3)If not able to provide expected freshness --> 422 with a code LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE

cc @javier-carrocalabor

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another difference with location-verification is that in this case we cannot return a lastLocationTime indicating how old is the more recent location

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @bigludo7 ,

  • is maxAge parameter absent in case (2) ?
  • in case (3), there is a maxAge parameter for sure, and it can also be 0, am I right ?
    Thank you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @alpaycetin74

is maxAge parameter absent in case (2) ?
Humm.. It is fair to assume that yes.

in case (3), there is a maxAge parameter for sure, and it can also be 0, am I right ?
Yes - no doubt for this one !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bigludo7 ,
if I may summarize my view:

For location retrieval,
(1) Obviously a client error, 400 is appropriate.
It is possible to advise LOCATION_RETRIEVAL.MAXAGE_INVALID_ARGUMENT, but the implementer may not have full control over error messages created in case of syntactic errors - those may be created by software libraries.
(2) No location available, even if maxAge is absent (infinite).
404 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND is appropriate.
(3) maxAge condition (including 0) cannot be satisfied.
422 with code LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE is appropriate.

For location verification,
(1) same
(2) UNKNOWN without lastLocationTime
(3) UNKNOWN, may have lastLocationTime if system has it.
Thank you

bigludo7 and others added 2 commits April 25, 2024 12:31
Co-authored-by: Jose Luis Urien <joseluis.urienpinedo@telefonica.com>
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added @alpaycetin74 proposal for location-verification

Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @alpaycetin74 proposal

Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space line 36

@@ -37,6 +37,10 @@ info:

* **Max Age**: Maximum age of the location information which is accepted for the location retrieval (in seconds).

* Absence of maxAge means "any age" is acceptable for the client. In other words, this is like maxAge=infinite. In this case the system may still return lastLocationTime, if available.
* maxAge=0 means a fresh calculation is requested by the client.
If the system does not support it, or fresh location cannot be calculated at that time for any reason, the API response will be "unknown".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line has to be modified in any case, as the API cannot response "unknown"

Aligned following our discussion
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal following our discussion @jlurien @alpaycetin74

Fixed megalinter issue
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed megalinter issue

@bigludo7
Copy link
Collaborator Author

bigludo7 commented May 3, 2024

@alpaycetin74 @jlurien Kind reminder on this one. Is there still blocking point for you?

@@ -37,6 +37,9 @@ info:

* **Max Age**: Maximum age of the location information which is accepted for the location retrieval (in seconds).

* Absence of maxAge means "any age" is acceptable for the client. In other words, this is like maxAge=infinite. In this case the system may still return lastLocationTime, if available. If the system is not able to provide location a error 400 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND is send back.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text should refer to 404 instead of 400, I think.
I find 400 appropriate only if the maxAge is syntactically wrong.

$ref: '#/components/responses/Generic404'
$ref: '#/components/responses/RetrieveLocationNotFound404'
'422':
$ref: '#/components/responses/RetrieveLocationUnprocessableEntity404'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema needs to be renamed to RetrieveLocationUnprocessableEntity422

status: 404
code: LOCATION_RETRIEVAL.DEVICE_NOT_FOUND
message: 'The location server is not able to locate the mobile'
RetrieveLocationUnprocessableEntity404:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema needs to be renamed to RetrieveLocationUnprocessableEntity422

Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed issues identified by @alpaycetin74

@alpaycetin74
Copy link
Collaborator

LGTM, thank you.

@@ -37,6 +37,9 @@ info:

* **Max Age**: Maximum age of the location information which is accepted for the location retrieval (in seconds).

* Absence of maxAge means "any age" is acceptable for the client. In other words, this is like maxAge=infinite. In this case the system may still return lastLocationTime, if available. If the system is not able to provide location a error 404 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND is send back.
* maxAge=0 means a fresh calculation is requested by the client. If the system is not able to provide fresh location (current time - location time > maxDate) an error 422 with code LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE is send back.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "maxDate"? If you mean "maxAge" I wouldn't include the formula, as it is confusing: current time - location time > 0 ==> current time > location time, which is always true.

code/API_definitions/location-verification.yaml Outdated Show resolved Hide resolved
bigludo7 and others added 2 commits May 6, 2024 13:44
Co-authored-by: Jose Luis Urien <joseluis.urienpinedo@telefonica.com>
Removal formula for UNABLE_TO_FULFILL_MAX_AGE
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removal formula for UNABLE_TO_FULFILL_MAX_AGE

@jlurien jlurien requested a review from alpaycetin74 May 7, 2024 07:26
Copy link
Collaborator

@alpaycetin74 alpaycetin74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@bigludo7 bigludo7 merged commit f7c5110 into main May 7, 2024
1 check passed
@bigludo7 bigludo7 deleted the bigLudo7---maxAge branch May 7, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantics of the absence of 'maxAge'
3 participants